-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enchancement: use Arc + Mutex for dbpool instance instead clone() #5037
base: main
Are you sure you want to change the base?
Conversation
Isn't it already a Line 170 in 040e2a7
Wrapping another around it again doesn't seem like a good option i think, but i might be wrong @dani-garcia ? |
it's a DbConnInner wrapped not a DbPool |
Still i think this is a bad idea. Ill do some tests. |
And it is.. I tested this by calling
hey -c 10 -n 100 -m POST \
-H 'authorization: Bearer eyJ0eX....Cw' \
-d '{"masterPasswordHash":"N30....7s4="}' \
http://127.0.0.1/api/two-factor/get-duo
Summary:
Total: 88.4836 secs
Slowest: 10.6611 secs
Fastest: 0.8882 secs
Average: 8.4133 secs
Requests/sec: 1.1302
Response time histogram:
0.888 [1] |■
1.866 [1] |■
2.843 [1] |■
3.820 [1] |■
4.797 [1] |■
5.775 [1] |■
6.752 [2] |■
7.729 [2] |■
8.707 [16] |■■■■■■■■■■■
9.684 [60] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
10.661 [14] |■■■■■■■■■
Latency distribution:
10% in 7.9186 secs
25% in 8.0089 secs
50% in 8.8373 secs
75% in 8.8653 secs
90% in 9.7137 secs
95% in 9.7540 secs
99% in 10.6611 secs
Details (average, fastest, slowest):
DNS+dialup: 0.0000 secs, 0.8882 secs, 10.6611 secs
DNS-lookup: 0.0000 secs, 0.0000 secs, 0.0000 secs
req write: 0.0000 secs, 0.0000 secs, 0.0005 secs
resp wait: 8.4131 secs, 0.8870 secs, 10.6609 secs
resp read: 0.0001 secs, 0.0001 secs, 0.0005 secs
Status code distribution:
[200] 100 responses Current main branch: hey -c 10 -n 100 -m POST \
-H 'authorization: Bearer eyJ0eX....Cw' \
-d '{"masterPasswordHash":"N30....7s4="}' \
http://127.0.0.1/api/two-factor/get-duo
Summary:
Total: 13.1764 secs
Slowest: 1.6208 secs
Fastest: 0.9380 secs
Average: 1.2475 secs
Requests/sec: 7.5893
Response time histogram:
0.938 [1] |■■
1.006 [0] |
1.075 [6] |■■■■■■■■■■■■
1.143 [18] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
1.211 [20] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
1.279 [19] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
1.348 [14] |■■■■■■■■■■■■■■■■■■■■■■■■■■■■
1.416 [11] |■■■■■■■■■■■■■■■■■■■■■■
1.484 [4] |■■■■■■■■
1.553 [5] |■■■■■■■■■■
1.621 [2] |■■■■
Latency distribution:
10% in 1.1011 secs
25% in 1.1493 secs
50% in 1.2363 secs
75% in 1.3284 secs
90% in 1.4567 secs
95% in 1.5047 secs
99% in 1.6208 secs
Details (average, fastest, slowest):
DNS+dialup: 0.0000 secs, 0.9380 secs, 1.6208 secs
DNS-lookup: 0.0000 secs, 0.0000 secs, 0.0000 secs
req write: 0.0000 secs, 0.0000 secs, 0.0003 secs
resp wait: 1.2472 secs, 0.9378 secs, 1.6207 secs
resp read: 0.0002 secs, 0.0000 secs, 0.0018 secs
Status code distribution:
[200] 100 responses |
The problem is that the concurrency doesn't work that well anymore because of the lock you set. This kinda defeats the usage of async also. |
Hm.. you right! lock() for read operation is not good! Refactor for RwLock Performance is the same |
But why as another layer around this? Why should we add another layer of Arc with a RwLock? Could you elaborate a bit more on this? |
It might help a bit in limiting the allocations maybe. But it might be minimal. |
First of all, yes, reduce the number of allocations. And secondly, it may be necessary to change the parameters of this pool from different places (yeah, right now this is not needed anywhere, but maybe in future... who knows) |
I think this should work. While i do not think we will add something which needs modifications to the pool or something, but have a bit less allocations is good too. If you could rebase on the current main branch that would be cool :). |
done |
Can you post a compared perf benchmark again ? |
hey usage
results for vaultwarden build from main branch:
results for vaultwarden build from changed branch with RwLock:
run on my local Mac Pro m1 |
I can confirm those results with tests i did. |
Is this considered a significant improvement? |
changes not for performance improvement |
Well, maybe there is a very very small performance gain, but not one measurable. Since less allocation need to be made, less memory is used and less CPU cycles to perform it. |
The Also I don't think the That said, this seems like premature optimization to me. |
I have the same thoughts. I tested it to see if it would lower the amount of allocations and memory, but it isn't detectable as far as i can see. Sure a bit less allocations, but not really an improvement with memory or speed. |
Hi, this is PR with minor changes - use Arc::clone instead .clone() instance of DbPool